Skip to content

Add better handling of deprecated configs#20565

Merged
MartinHjelmare merged 16 commits into
home-assistant:devfrom
rohankapoorcom:deprecated_configs
Feb 8, 2019
Merged

Add better handling of deprecated configs#20565
MartinHjelmare merged 16 commits into
home-assistant:devfrom
rohankapoorcom:deprecated_configs

Conversation

@rohankapoorcom
Copy link
Copy Markdown
Member

@rohankapoorcom rohankapoorcom commented Jan 29, 2019

Description:

Allow the ability to deprecate specific configuration keys with optional replacements and optional invalidation dates to make less breaking changes. Here's how it works:

  • cv.deprecated now takes additional parameters replacement_key, invalidation_version, default
  • If all of those are provided, it checks if this version of Home Assistant has crossed the invalidation_version and if not raises a warning indicating what the replacement key to use is.
  • It will then grab the value from the deprecated parameter and insert it into replacement parameter, replacing the original.
  • If it has crossed the invalidation_version it raises vol.Invalid with the same message indicating how to resolve the problem.

I've demonstrated with the update_interval configuration key being replaced with the scan_interval configuration key in the freedns component.

This needs a few more tests before this can be merged. A good next step would be to surface this in the UI.

Related issue (if applicable): Spawned from discussion in #20526.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

Comment thread homeassistant/components/freedns.py Outdated
Comment thread tests/helpers/test_config_validation.py Outdated
warning = ("The '{key}' option (with value '{value}') is deprecated,"
" please remove it from your configuration.")

def check_for_invalid_version(value: Optional[str]):
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point, it may be worth pulling this out of an inner function and making it a decorator that could be used in other things besides configuration expiration.

Comment thread homeassistant/helpers/config_validation.py Outdated
@rohankapoorcom
Copy link
Copy Markdown
Member Author

I added some fairly extensive unit tests (and fixed a couple of bugs along the way). I also added some documentation explaining the behavior. As far as I can tell, this is good to go.

Comment thread homeassistant/helpers/config_validation.py Outdated
Comment thread homeassistant/helpers/logging.py Outdated
Comment thread homeassistant/helpers/logging.py Outdated
@rohankapoorcom
Copy link
Copy Markdown
Member Author

@balloob @MartinHjelmare all comments have been addressed, is there anything else you'd like to see changed or do you think this is ready to go? It'd be nice to have available with 0.87 so that we can start cleaning up some things.

@MartinHjelmare
Copy link
Copy Markdown
Member

Would anything benefit in 0.87? I think we should just merge to dev when it's ready without tagging.

@rohankapoorcom
Copy link
Copy Markdown
Member Author

My thinking is that it would give us the starting version to use to deprecate things at, but you're right, we could start that with the next version.

@balloob
Copy link
Copy Markdown
Member

balloob commented Feb 2, 2019

I prefer the next version because we just had a pretty rocky release.

Comment thread homeassistant/helpers/config_validation.py Outdated
Comment thread homeassistant/helpers/config_validation.py
Comment thread homeassistant/helpers/config_validation.py
Comment thread homeassistant/helpers/config_validation.py
Comment thread homeassistant/helpers/config_validation.py Outdated
Comment thread homeassistant/helpers/config_validation.py Outdated
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I've read through everything finally. Some more comments below.

Comment thread homeassistant/helpers/config_validation.py Outdated
Comment thread homeassistant/helpers/config_validation.py Outdated
Comment thread tests/helpers/test_config_validation.py Outdated
Comment thread tests/helpers/test_config_validation.py Outdated
Comment thread tests/helpers/test_config_validation.py
Comment thread tests/helpers/test_config_validation.py Outdated
@rohankapoorcom
Copy link
Copy Markdown
Member Author

Rebased and cleaned up merged conflicts. @MartinHjelmare @balloob any further concerns with this?

@MartinHjelmare
Copy link
Copy Markdown
Member

This is great!

@MartinHjelmare MartinHjelmare changed the title RFC: Add better handling of deprecated configs Add better handling of deprecated configs Feb 8, 2019
@MartinHjelmare MartinHjelmare merged commit d5fad33 into home-assistant:dev Feb 8, 2019
@ghost ghost removed the in progress label Feb 8, 2019
@rohankapoorcom rohankapoorcom deleted the deprecated_configs branch February 8, 2019 15:35
@balloob balloob mentioned this pull request Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants